Add multithreaded tests for cleanup of handler when stopping search

[saksham-chawla]

What
This change introduces a suite of new multithreaded tests to verify the robustness of the StopFindService mechanism, addressing the potential for race conditions identified in the issue eclipse-score#86. It specifically targets corner cases related to handler cleanup that could lead to use-after-free errors when StopFindService is called from within a FindServiceHandler or from multiple threads concurrently.

The original implementation had a conceptual flaw where a FindServiceHandler could be destroyed prematurely if StopFindService was called from within the handler itself. While this was fixed with a "late removal" mechanism, the existing test coverage was insufficient to guarantee the fix was robust under various concurrent scenarios.

This PR adds four new standalone test applications, each targeting a specific corner case:

1. find_stop_find_test
   -Scenario: This is the fundamental re-entrancy test. It starts a service discovery, and when the FindServiceHandler is
   invoked, it immediately calls StopFindService on the very handle that triggered the callback.
   - Purpose: This test directly verifies that the "late removal" of the handler is working correctly. If the handler's resources
   were deallocated immediately upon the StopFindService call, the handler would crash before it could finish executing. The
   success of this test proves that the framework safely waits for the handler to complete before cleanup, preventing a use-
   after-free error.

2. find_long_running_handler_test:
   - Scenario: The FindServiceHandler calls StopFindService on its own handle and then continues to execute for a
   significant duration (by sleeping) before returning.
   - Purpose: Directly stresses the "late removal" mechanism. It ensures that the handler's resources remain valid for its
   entire lifetime, even after the stop request has been processed, preventing use-after-free errors.

3. find_inter_stop_test:
   - Scenario: Two independent service discovery operations are active. The handler for xpad/cp60/MapApiLanesStamped
   is triggered and immediately calls StopFindService on the handle for xpad/cp60/MapApiLanesStamped_B.
   - Purpose: Checks for unintended side effects or interference between separate, concurrent discovery operations,
   ensuring that stopping one does not corrupt the state of another.

4. find_concurrent_stop_test:
   - Scenario: Tests thread-safety by having two threads (the FindServiceHandler and a dedicated stopper_thread) call
   StopFindService on the same handle concurrently.
   - Purpose: Verifies that the internal state management of the discovery process is protected against race conditions
   and that the system handles simultaneous stop requests gracefully without deadlocking or crashing.

Why
To prevent regressions and ensure the stability of the service discovery framework, we need comprehensive tests that cover critical race conditions. These tests simulate aggressive, concurrent usage patterns that were not previously covered, increasing confidence in the "late removal" fix for handlers and the overall thread-safety of the mw::com runtime.

---

Add multithreaded tests for cleanup of handler when stopping search

This pull request introduces a suite of new multithreaded tests to enhance the robustness of the StopFindService mechanism. It specifically addresses potential race conditions and use-after-free errors related to handler cleanup when StopFindService is invoked from within a FindServiceHandler or concurrently from multiple threads.

The new tests cover critical scenarios such as re-entrancy (handler stopping its own discovery), long-running handlers after a stop request, inter-discovery stopping (one handler stopping another discovery), and concurrent stop requests from multiple threads. These additions aim to prevent regressions and increase confidence in the thread-safety and "late removal" fix for handlers within the mw::com runtime.

Architecture Diagram:

flowchart TD
    subgraph Test_Applications
        A["find_stop_find_test (+116 lines)"]
        B["find_long_running_handler_test (+120 lines)"]
        C["find_inter_stop_test (+159 lines)"]
        D["find_concurrent_stop_test (+136 lines)"]
    end

    subgraph Test_Framework
        E["ProxyBaseTest (+215 lines)"]
    end

    subgraph Core_Communication_Module
        F["ProxyBase"]
        G["ServiceDiscovery"]
        H["Runtime"]
        I["ServiceSkeleton"]
    end

    subgraph Configuration_and_Build
        J[/"'mw_com_config.json (+34 lines)'"/]
        K["BUILD (+51 lines)"]
    end

    A -->|uses| H
    A -->|uses| F
    A -->|interacts with| I

    B -->|uses| H
    B -->|uses| F
    B -->|interacts with| I

    C -->|uses| H
    C -->|uses| F
    C -->|interacts with| I

    D -->|uses| H
    D -->|uses| F
    D -->|interacts with| I

    E -->|tests| F
    E -->|mocks| G
    H -->|uses| G
    H -->|reads| J
    K -->|compiles| A
    K -->|compiles| B
    K -->|compiles| C
    K -->|compiles| D
    K -->|compiles| E
    K -->|configures| J

Loading

---

This summary was automatically generated by @propel-code-bot

[propel-code-bot]

Critical config JSON breakage and multiple concurrency/test reliability gaps must be addressed before merge.

Status: Action Required | Risk: High | Readiness: 3/10 (Needs Significant Work)

Issues Identified & Suggestions

• Fix config JSON and use packaged path:mw_com_config.json,find_stop_find_test.cpp
• Harden tests: timeouts, promise guard, ptr capture, dedup, :find_inter_stop_test.cpp,find_stop_find_test.cpp,find_long_running_handler_test.cpp,find_concurrent_stop_test.cpp

Review Details

📁 7 files reviewed | 💬 10 comments

👍 / 👎 individual comments to help improve reviews for you

[propel-code-bot]

[Logic] Malformed JSON: the diff closes the first serviceInstances entry and its parent array too early, so the second service instance (xpad/cp60/MapApiLanesStamped_B) ends up as a sibling of serviceInstances at the root level rather than being inside it. Looking at the resulting file:

  "serviceInstances": [
    { /* first instance */ }
  ]
},
{
  "instanceSpecifier": "xpad/cp60/MapApiLanesStamped_B",
  ...
}

This produces two top-level objects separated by a comma (invalid JSON), and even if a parser tolerates it, the second instance is not in the serviceInstances array. The config parser expects all instances inside that array.

Fix: ensure the second service instance is a sibling element within the serviceInstances array:

  "serviceInstances": [
    {
      "instanceSpecifier": "xpad/cp60/MapApiLanesStamped",
      ...
    },
    {
      "instanceSpecifier": "xpad/cp60/MapApiLanesStamped_B",
      ...
    }
  ]
}

Context for Agents

Malformed JSON: the diff closes the first `serviceInstances` entry and its parent array too early, so the second service instance (`xpad/cp60/MapApiLanesStamped_B`) ends up as a sibling of `serviceInstances` at the root level rather than being inside it. Looking at the resulting file:

```json
  "serviceInstances": [
    { /* first instance */ }
  ]
},
{
  "instanceSpecifier": "xpad/cp60/MapApiLanesStamped_B",
  ...
}
```

This produces two top-level objects separated by a comma (invalid JSON), and even if a parser tolerates it, the second instance is not in the `serviceInstances` array. The config parser expects all instances inside that array.

Fix: ensure the second service instance is a sibling element within the `serviceInstances` array:
```json
  "serviceInstances": [
    {
      "instanceSpecifier": "xpad/cp60/MapApiLanesStamped",
      ...
    },
    {
      "instanceSpecifier": "xpad/cp60/MapApiLanesStamped_B",
      ...
    }
  ]
}
```

File: score/mw/com/example/ipc_bridge/etc/mw_com_config.json
Line: 61

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.00
• Likelihood Score: 0/5
• Suggested Action: keep
• Tool Calls Used: 2

Reasoning

Failed to parse validation response

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/etc/mw_com_config.json

1: {
2:   "serviceTypes": [
3:     {
4:       "serviceTypeName": "/bmw/adp/MapApiLanesStamped",
5:       "version": {
6:         "major": 1,
7:         "minor": 0
8:       },
9:       "bindings": [
10:         {
...

2. File Snapshot (BASE)

• File: score/mw/com/example/ipc_bridge/etc/mw_com_config.json

1: {
2:   "serviceTypes": [
3:     {
4:       "serviceTypeName": "/bmw/adp/MapApiLanesStamped",
5:       "version": {
6:         "major": 1,
7:         "minor": 0
8:       },
9:       "bindings": [
10:         {
...

[propel-code-bot]

[Logic] Race condition in find_handle_B_ptr capture: the shared_ptr is captured by value into the lambda at this point, when it is still null (default-constructed on line 95). The assignment at line 143 (find_handle_B_ptr = std::make_shared<...>(...)) updates the local variable in main(), but the lambda already holds a copy of the original null pointer. As a result, if (find_handle_B_ptr) on line 104 will always be false, and StopFindService for service B will never be called from handler A — the test silently passes without testing anything.

Fix: capture by reference instead:

auto find_service_handler_A =
    [&handler_A_finished_promise, &find_handle_B_ptr](

Note that this also requires ensuring find_handle_B_ptr is assigned before handler A fires. Consider using a synchronization mechanism (e.g., a std::promise/std::future or barrier) to guarantee handler A does not execute until find_handle_B_ptr has been set.

Context for Agents

Race condition in `find_handle_B_ptr` capture: the `shared_ptr` is captured **by value** into the lambda at this point, when it is still null (default-constructed on line 95). The assignment at line 143 (`find_handle_B_ptr = std::make_shared<...>(...)`) updates the local variable in `main()`, but the lambda already holds a copy of the original null pointer. As a result, `if (find_handle_B_ptr)` on line 104 will **always** be false, and `StopFindService` for service B will never be called from handler A — the test silently passes without testing anything.

Fix: capture by reference instead:
```cpp
auto find_service_handler_A =
    [&handler_A_finished_promise, &find_handle_B_ptr](
```
Note that this also requires ensuring `find_handle_B_ptr` is assigned before handler A fires. Consider using a synchronization mechanism (e.g., a `std::promise`/`std::future` or barrier) to guarantee handler A does not execute until `find_handle_B_ptr` has been set.

File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp
Line: 99

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.99
• Likelihood Score: 5/5
• Likelihood Reason: This is a definitive bug, not a race condition possibility. The shared_ptr is captured by value when it is null. The lambda's copy will always be null regardless of timing. The if-check on line 104 will always evaluate to false, meaning StopFindService for B is never called from handler A. The test silently passes without testing its stated purpose every single time it runs.
• Suggested Action: keep
• Tool Calls Used: 2

Reasoning

The comment correctly identifies a clear C++ lambda capture bug in newly added code:

1. Capture by value of null shared_ptr: On line 95, find_handle_B_ptr is default-constructed (null). On line 99, it's captured by value (find_handle_B_ptr without &), creating a copy in the lambda's closure that is null at that point.

2. Late assignment doesn't propagate: On line 143, find_handle_B_ptr = std::make_shared<...>(...) updates only the local variable in main(). The lambda holds its own independent copy which remains null forever.

3. Dead code path: Inside the lambda, if (find_handle_B_ptr) on line 104 will always be false. The StopFindService(*find_handle_B_ptr) call on line 106 will never execute.

4. Test silently passes without testing anything: The test's stated purpose (line 59-60) is to verify "calling StopFindService for one discovery operation from within the handler of another discovery operation works correctly." But that code path is never exercised due to this bug.

5. New file: This is a brand new file added in this PR, confirmed by the base commit returning "File not found." The issue is entirely introduced by this PR.

The comment's fix suggestion (capture by reference with &find_handle_B_ptr) is correct and actionable. The additional note about synchronization is also valid — there's a race between StartFindService triggering handler A (line 122) and the assignment on line 143. If handler A fires before line 143, even with capture by reference, the pointer would still be null.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp
• Lines: 93-145

93:     // We need to capture the handle for discovery B to stop it from handler A.
94:     // Since the handle is only created after StartFindService, we use a shared_ptr to allow late assignment.
95:     std::shared_ptr<score::mw::com::FindServiceHandle> find_handle_B_ptr;
96: 
97:     // 5. Define the handlers for each discovery.
98:     auto find_service_handler_A =
99:         [&handler_A_finished_promise, find_handle_B_ptr](
100:             const score::mw::com::ServiceHandleContainer<IpcBridgeProxy::HandleType>&,
101:             score::mw::com::FindServiceHandle) {
102:             std::cout << "HANDLER A: Service A found. Stopping discovery for service B...\n";
...

[propel-code-bot]

[Logic] Same issue as in find_long_running_handler_test.cpp: if the handler is invoked more than once (e.g., due to a second availability notification before StopFindService completes), service_found_promise.set_value() will throw std::future_error, crashing the test.

Fix: guard with std::call_once or an atomic flag to ensure set_value() is called at most once.

Context for Agents

Same issue as in `find_long_running_handler_test.cpp`: if the handler is invoked more than once (e.g., due to a second availability notification before `StopFindService` completes), `service_found_promise.set_value()` will throw `std::future_error`, crashing the test.

Fix: guard with `std::call_once` or an atomic flag to ensure `set_value()` is called at most once.

File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp
Line: 91

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.78
• Likelihood Score: 3/5
• Likelihood Reason: The handler is a callback for asynchronous service discovery. In a multithreaded IPC framework, a second availability notification could dispatch the handler again before StopFindService (called at line 90) takes effect, causing a double set_value() and a std::future_error crash. The likelihood depends on the framework's internal guarantees about handler serialization, but in a test explicitly designed to exercise race conditions, this is a plausible scenario.
• Suggested Action: keep
• Tool Calls Used: 4

Reasoning

The comment identifies a real potential issue in newly introduced code:

1. Accuracy: std::promise::set_value() is indeed specified to throw std::future_error with error code promise_already_satisfied if called on a promise whose shared state already has a stored value. This is a well-known C++ standard behavior.

2. Code Analysis: In find_stop_find_test.cpp, the handler at lines 84-92 captures service_found_promise by reference and calls set_value() unconditionally at line 91. The handler calls StopFindService at line 90 first, but there's a window where:

   • Thread A enters the handler, starts executing
   • Thread B (another notification) enters the handler concurrently before Thread A's StopFindService call takes effect
   • Both threads attempt set_value(), and the second one throws

3. Introduced by this PR: This is entirely new code added in this PR (the file goes from 0 lines to 116 lines), so the issue is squarely within the PR's scope.

4. Actionable: The fix suggestion is concrete - use std::call_once or an atomic flag to guard the set_value() call. This is a well-known pattern for one-shot promise fulfillment in concurrent code.

5. Cross-reference: The same pattern exists in find_long_running_handler_test.cpp (line 96) and find_concurrent_stop_test.cpp (lines 94, 99), confirming this is a systemic pattern in the PR. Interestingly, find_inter_stop_test.cpp has the same issue at line 110.

The only uncertainty is whether the framework guarantees that once StopFindService is called from within a handler, no further handler invocations can occur. If the framework provides such a guarantee, the issue would never manifest. However, given that these tests explicitly exercise race conditions and concurrent scenarios, defensive coding against double invocation is prudent.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

4. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  * See the NOTICE file(s) distributed with this work for additional
4:  * information regarding copyright ownership.
5:  *
6:  * This program and the accompanying materials are made available under the
7:  * terms of the Apache License Version 2.0 which is available at
8:  * https://www.apache.org/licenses/LICENSE-2.0
9:  *
10:  * SPDX-License-Identifier: Apache-2.0
...

[propel-code-bot]

[Reliability] This wait is unbounded; if the skeleton fails to offer the service or the discovery callback never fires, the test will hang indefinitely. Store the future and use a timeout so the test fails and cleans up instead of blocking forever.

Suggested change

	handler_finished_promise.get_future().wait();
	auto handler_future = handler_finished_promise.get_future();
	if (handler_future.wait_for(5s) != std::future_status::ready) {
	std::cerr << "HANDLER: timeout\n";
	test_complete_promise.set_value();
	skeleton_thread.join();
	return EXIT_FAILURE;
	}

Context for Agents

This wait is unbounded; if the skeleton fails to offer the service or the discovery callback never fires, the test will hang indefinitely. Store the future and use a timeout so the test fails and cleans up instead of blocking forever.

```suggestion
auto handler_future = handler_finished_promise.get_future();
if (handler_future.wait_for(5s) != std::future_status::ready) {
    std::cerr << "HANDLER: timeout\n";
    test_complete_promise.set_value();
    skeleton_thread.join();
    return EXIT_FAILURE;
}
```

File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp
Line: 111

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.90
• Likelihood Score: 3/5
• Likelihood Reason: The skeleton can silently fail (e.g., configuration issues, IPC infrastructure unavailable), causing the discovery callback to never fire and the test to hang indefinitely. This is a plausible failure mode in CI or development environments, though in a properly configured environment the service discovery usually works.
• Suggested Action: keep
• Tool Calls Used: 5

Reasoning

The comment correctly identifies that handler_finished_promise.get_future().wait() at line 111 is an unbounded blocking call. The promise is only fulfilled inside the discovery callback (line 96), which depends on: (1) the skeleton successfully creating and offering the service in the background thread, and (2) the service discovery mechanism actually invoking the callback.

The RunSkeleton function has two silent failure paths (lines 35-38 and lines 42-46) where it prints to stderr and returns without notifying the main thread. If either fails, the skeleton thread exits, the service is never offered, the discovery callback never fires, and the main thread blocks forever at line 111.

These are cc_binary targets (not cc_test), so they don't benefit from Bazel's built-in test timeout mechanism. The test would genuinely hang forever with no indication of what went wrong.

The comment provides a concrete, actionable fix using wait_for with a timeout, which would allow the test to fail gracefully and provide diagnostic output. The suggestion correctly stores the future separately (since get_future() can only be called once per promise) and includes proper cleanup of the skeleton thread.

This is entirely new code introduced by this PR, not legacy debt.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  * See the NOTICE file(s) distributed with this work for additional
4:  * information regarding copyright ownership.
5:  *
6:  * This program and the accompanying materials are made available under the
7:  * terms of the Apache License Version 2.0 which is available at
8:  * https://www.apache.org/licenses/LICENSE-2.0
9:  *
10:  * SPDX-License-Identifier: Apache-2.0
...

4. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/BUILD

1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

[propel-code-bot]

[Reliability] This wait is unbounded; if the handler is never invoked (e.g., service discovery never matches or the skeleton fails), the test will hang. Use wait_for with a timeout and exit with failure on timeout.

Suggested change

	service_found_future.wait();
	if (service_found_future.wait_for(5s) != std::future_status::ready) {
	std::cerr << "PROXY: timeout waiting for handler\n";
	test_complete_promise.set_value();
	skeleton_thread.join();
	return EXIT_FAILURE;
	}

Context for Agents

This wait is unbounded; if the handler is never invoked (e.g., service discovery never matches or the skeleton fails), the test will hang. Use `wait_for` with a timeout and exit with failure on timeout.

```suggestion
if (service_found_future.wait_for(5s) != std::future_status::ready) {
    std::cerr << "PROXY: timeout waiting for handler\n";
    test_complete_promise.set_value();
    skeleton_thread.join();
    return EXIT_FAILURE;
}
```

File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp
Line: 107

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.93
• Likelihood Score: 4/5
• Likelihood Reason: The unbounded wait will cause the test to hang if the skeleton fails to create or offer the service (lines 36-39, 44-47 in RunSkeleton return early without ever triggering service discovery), if service discovery never matches, or if any configuration issue prevents the handler from being invoked. Since this is an integration test relying on IPC infrastructure, such failures are realistic.
• Suggested Action: keep
• Tool Calls Used: 3

Reasoning

The comment correctly identifies that service_found_future.wait() at line 107 is an unbounded wait on a future. This is newly introduced code (the entire file is new in this PR).

Analysis of the hang scenario:

1. The skeleton thread's RunSkeleton function can return early without waiting on test_complete_future if skeleton creation fails (line 36-39) or if OfferService() fails (line 44-47). In either case, no service is offered, so service discovery never matches, and the find_service_handler lambda is never called.
2. If the handler is never called, service_found_promise.set_value() (line 91) is never invoked.
3. Therefore service_found_future.wait() at line 107 blocks forever, causing the test to hang.

Consistency check: All other newly added test files in this PR (find_concurrent_stop_test.cpp:126, find_inter_stop_test.cpp:146, find_long_running_handler_test.cpp:111) also have the same unbounded wait() pattern on similar futures. This is a systemic issue across the new test files.

Actionability: The comment provides a concrete code suggestion using wait_for(5s) with proper cleanup and error reporting on timeout. The suggestion correctly handles the test_complete_promise.set_value() call and skeleton_thread.join() before returning EXIT_FAILURE.

Diff responsibility: This is entirely new code introduced by this PR, so the issue is squarely within scope.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

[propel-code-bot]

[Reliability] This wait can block forever if the handler never runs (e.g., service discovery never matches or the skeleton fails early at lines 33-44 without notifying the main thread). Add a timeout and fail/cleanup on timeout so the test doesn't hang CI.

Note: the stopper thread (line 117) also has an unbounded release_stopper_future.wait() that would hang in the same scenario, so its cleanup needs to be addressed too (e.g., by also fulfilling release_stopper_promise in the timeout path before joining the stopper thread).

Suggested change

	handler_finished_future.wait();
	if (handler_finished_future.wait_for(5s) != std::future_status::ready) {
	std::cerr << "HANDLER: timeout\n";
	// Also unblock stopper_thread which is waiting on release_stopper_future
	release_stopper_promise.set_value();
	stopper_thread.join();
	test_complete_promise.set_value();
	skeleton_thread.join();
	return EXIT_FAILURE;
	}

Context for Agents

This wait can block forever if the handler never runs (e.g., service discovery never matches or the skeleton fails early at lines 33-44 without notifying the main thread). Add a timeout and fail/cleanup on timeout so the test doesn't hang CI.

Note: the stopper thread (line 117) also has an unbounded `release_stopper_future.wait()` that would hang in the same scenario, so its cleanup needs to be addressed too (e.g., by also fulfilling `release_stopper_promise` in the timeout path before joining the stopper thread).

```suggestion
if (handler_finished_future.wait_for(5s) != std::future_status::ready) {
    std::cerr << "HANDLER: timeout\n";
    // Also unblock stopper_thread which is waiting on release_stopper_future
    release_stopper_promise.set_value();
    stopper_thread.join();
    test_complete_promise.set_value();
    skeleton_thread.join();
    return EXIT_FAILURE;
}
```

File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp
Line: 126

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.85
• Likelihood Score: 3/5
• Likelihood Reason: The scenario (handler never firing) is plausible - if the skeleton fails to create or offer the service (lines 33-44), it returns early without signaling failure to the main thread, leaving handler_finished_future.wait() and the stopper thread's release_stopper_future.wait() blocked indefinitely. In a healthy CI environment with both processes running correctly, service discovery should succeed, but infrastructure failures or flaky IPC could trigger this.
• Suggested Action: modify
• Tool Calls Used: 5

Reasoning

The comment correctly identifies that handler_finished_future.wait() on line 126 is an unbounded wait that can block forever if the service discovery handler is never invoked.

Evidence for validity:

1. The issue is real: If RunSkeleton fails at either Create() (line 32) or OfferService() (line 40), it returns early without notifying the main thread. The service is never offered, so the find_service_handler lambda is never called, meaning handler_finished_promise is never set, and handler_finished_future.wait() blocks forever.

2. No external timeout protection: The BUILD file (line 74-85) defines these as cc_binary targets, not cc_test with a Bazel timeout. There's no test runner timeout configured at the build level.

3. Newly introduced by this PR: The file is entirely new (all lines are added), so this is not legacy debt.

4. Consistent pattern across the PR: All four new test files (find_inter_stop_test.cpp:146, find_long_running_handler_test.cpp:111, find_stop_find_test.cpp:107) have the same unbounded wait pattern. None use timeouts.

5. Actionable: The comment provides a concrete code suggestion using wait_for(5s) with proper cleanup and failure reporting.

Minor caveat: The suggested fix is slightly incomplete - if the handler never fires, the stopper_thread (line 115) is also blocked on release_stopper_future.wait() (line 117) and would need to be handled (e.g., the stopper_thread cannot be joined until its future is also resolved or the thread is detached). But this is a refinement issue, not a reason to reject the comment - the core observation about the hanging CI risk is correct and actionable.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  * See the NOTICE file(s) distributed with this work for additional
4:  * information regarding copyright ownership.
5:  *
6:  * This program and the accompanying materials are made available under the
7:  * terms of the Apache License Version 2.0 which is available at
8:  * https://www.apache.org/licenses/LICENSE-2.0
9:  *
10:  * SPDX-License-Identifier: Apache-2.0
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

4. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/BUILD

1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

[propel-code-bot]

[Reliability] This wait is unbounded; if handler A never fires, the program will block forever. Use a timeout and exit with failure on timeout so this test doesn't hang.

Suggested change

	handler_A_finished_future.wait();
	if (handler_A_finished_future.wait_for(5s) != std::future_status::ready) {
	std::cerr << "HANDLER A: timeout\n";
	shutdown_flag = true;
	skeleton_A_thread.join();
	skeleton_B_thread.join();
	return EXIT_FAILURE;
	}

Context for Agents

This wait is unbounded; if handler A never fires, the program will block forever. Use a timeout and exit with failure on timeout so this test doesn't hang.

```suggestion
if (handler_A_finished_future.wait_for(5s) != std::future_status::ready) {
    std::cerr << "HANDLER A: timeout\n";
    shutdown_flag = true;
    skeleton_A_thread.join();
    skeleton_B_thread.join();
    return EXIT_FAILURE;
}
```

File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp
Line: 146

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.92
• Likelihood Score: 3/5
• Likelihood Reason: In normal operation, handler A should fire quickly when service A is discovered. However, in CI environments or under resource pressure, service discovery could fail silently, causing the test to hang indefinitely. An unbounded wait in a test is a well-known anti-pattern that turns test failures into test hangs, which are harder to debug.
• Suggested Action: keep
• Tool Calls Used: 4

Reasoning

The comment correctly identifies that handler_A_finished_future.wait() on line 146 is an unbounded wait that will block forever if handler A never fires. This is newly introduced code (the entire file is new in this PR).

The handler_A_finished_promise is set in the find_service_handler_A lambda (line 110), which only fires when service A is discovered. If the skeleton fails to start, the service discovery never triggers, or there's any IPC/infrastructure issue, the promise is never fulfilled and the test hangs forever.

The suggestion to use wait_for with a timeout is the standard best practice for test code. The concrete code suggestion provides proper cleanup (setting shutdown_flag, joining threads, returning EXIT_FAILURE).

The same unbounded wait pattern exists in all other new test files in this PR (find_concurrent_stop_test.cpp:126, find_long_running_handler_test.cpp:111, find_stop_find_test.cpp:107), making this a systemic issue in the PR.

Minor note: the suggested cleanup doesn't call StopFindService for active discoveries before cleanup, which the normal path does at line 151, but the core recommendation is sound and actionable.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  * See the NOTICE file(s) distributed with this work for additional
4:  * information regarding copyright ownership.
5:  *
6:  * This program and the accompanying materials are made available under the
7:  * terms of the Apache License Version 2.0 which is available at
8:  * https://www.apache.org/licenses/LICENSE-2.0
9:  *
10:  * SPDX-License-Identifier: Apache-2.0
...

4. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/BUILD

1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

[propel-code-bot]

[Maintainability] [CodeDuplication] The RunSkeleton function is copied almost verbatim across find_stop_find_test.cpp, find_long_running_handler_test.cpp, and find_concurrent_stop_test.cpp (same signature using std::future<void>, same body). A fourth variant in find_inter_stop_test.cpp uses std::atomic<bool> but is otherwise the same pattern.

Consider extracting RunSkeleton into a shared helper header (e.g., test_skeleton_helper.h) to avoid maintaining four copies.

Context for Agents

[CodeDuplication] The `RunSkeleton` function is copied almost verbatim across `find_stop_find_test.cpp`, `find_long_running_handler_test.cpp`, and `find_concurrent_stop_test.cpp` (same signature using `std::future<void>`, same body). A fourth variant in `find_inter_stop_test.cpp` uses `std::atomic<bool>` but is otherwise the same pattern.

Consider extracting `RunSkeleton` into a shared helper header (e.g., `test_skeleton_helper.h`) to avoid maintaining four copies.

File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp
Line: 33

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.95
• Likelihood Score: 4/5
• Likelihood Reason: Four copies of very similar code introduced in a single PR will create real maintenance burden. Any bug fix or signature change to RunSkeleton must be replicated across all files.
• Suggested Action: keep
• Tool Calls Used: 7

Reasoning

The comment accurately identifies code duplication introduced by this PR:

1. Three verbatim copies: RunSkeleton in find_stop_find_test.cpp, find_long_running_handler_test.cpp, and find_concurrent_stop_test.cpp are byte-for-byte identical — same signature (const InstanceSpecifier&, std::future<void>), same body (Create skeleton, OfferService, wait on future, StopOfferService).

2. Fourth variant: find_inter_stop_test.cpp uses std::atomic<bool>& instead of std::future<void> and polls in a while-loop, with slightly different log messages. The comment correctly notes this difference.

3. Newly introduced: All four files are new in this PR (confirmed by checking base commit — none existed before). This duplication is entirely introduced by this PR, not legacy debt.

4. Actionable: The comment provides a concrete suggestion — extract RunSkeleton into a shared helper header like test_skeleton_helper.h. This is straightforward to implement and would eliminate maintaining three identical copies.

5. Realistic concern: These are standalone test binaries, so a shared header is a natural solution. The three identical variants can be trivially shared; the atomic variant could either be a second overload or a template parameterized on the stop mechanism.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

[propel-code-bot]

[Maintainability] std::shared_ptr/std::make_shared are used below but <memory> isn’t included. Add #include <memory> so this doesn’t rely on transitive includes and break on header changes.

Context for Agents

`std::shared_ptr`/`std::make_shared` are used below but `<memory>` isn’t included. Add `#include <memory>` so this doesn’t rely on transitive includes and break on header changes.

File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp
Line: 95

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.97
• Likelihood Score: 4/5
• Likelihood Reason: The file uses std::shared_ptr (line 95) and std::make_shared (line 143) without including <memory>. It currently compiles only via transitive includes from other headers. A future header refactor or toolchain change could easily break this.
• Suggested Action: keep
• Tool Calls Used: 2

Reasoning

The comment is accurate and actionable:

1. Fact check - std::shared_ptr/std::make_shared usage: Line 95 uses std::shared_ptr<score::mw::com::FindServiceHandle> and line 143 uses std::make_shared<score::mw::com::FindServiceHandle>(...). Both are clearly present in the new file.

2. Fact check - <memory> not included: The includes (lines 15-25) are: "datatype.h", "score/mw/com/runtime.h", "score/mw/com/types.h", <score/assert.hpp>, <atomic>, <chrono>, <future>, <iostream>, <thread>. There is no #include <memory>.

3. Introduced by this PR: This is an entirely new file (all 159 lines are added), so this issue is definitively introduced by this PR.

4. Actionable: The fix is clear and specific — add #include <memory> to the include list.

5. Valid concern: Relying on transitive includes is a well-known C++ anti-pattern. The C++ standard does not guarantee that any of the included headers will transitively include <memory>, so this could break with a different compiler, standard library version, or header refactor.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp
• Lines: 1-30

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

[propel-code-bot]

[Maintainability] Runtime initialization is still hard-coded to /etc/mw_com_config.json, even though the BUILD target now ships a configuration file via data = ["etc/mw_com_config.json"]. That means these binaries continue to depend on whatever happens to be installed on the host filesystem, making the tests non-hermetic and ignoring the config bundled with the artifact. To keep the dependency boundary self-contained, obtain the config path from the runfiles (or accept it via argv/env) and point RuntimeConfiguration at the packaged file; otherwise the new configuration changes will never be seen when the binaries run in CI or on developer machines without root-level /etc access. Apply the same fix to the other newly added test binaries.

Context for Agents

Runtime initialization is still hard-coded to `/etc/mw_com_config.json`, even though the BUILD target now ships a configuration file via `data = ["etc/mw_com_config.json"]`. That means these binaries continue to depend on whatever happens to be installed on the host filesystem, making the tests non-hermetic and ignoring the config bundled with the artifact. To keep the dependency boundary self-contained, obtain the config path from the runfiles (or accept it via argv/env) and point `RuntimeConfiguration` at the packaged file; otherwise the new configuration changes will never be seen when the binaries run in CI or on developer machines without root-level `/etc` access. Apply the same fix to the other newly added test binaries.

File: score/mw/com/example/ipc_bridge/find_stop_find_test.cpp
Line: 63

Chain of Thought

• Validation Decision: accepted
• Confidence: 0.88
• Likelihood Score: 4/5
• Likelihood Reason: In any CI or sandboxed Bazel execution environment without root-level /etc access, the hardcoded path will fail to find the config file, even though it's bundled with the binary via data. The existing main.cpp already avoids this by accepting the path via command-line args. This issue will surface on developer machines and CI runners that don't have a host-installed /etc/mw_com_config.json.
• Suggested Action: keep
• Tool Calls Used: 7

Reasoning

The comment is accurate and actionable:

1. Accurate identification of the problem: All four newly added test binaries hardcode "/etc/mw_com_config.json" as the config path, while the BUILD file bundles etc/mw_com_config.json via the data attribute. In Bazel, data files are made available through the runfiles tree, not at absolute filesystem paths like /etc/.

2. Introduced by this PR: All four test files and their BUILD entries are newly added in this PR, so this is not pre-existing debt.

3. Existing pattern supports the fix: The pre-existing main.cpp in the same directory takes the config path via command-line argument (--service_instance_manifest), demonstrating the codebase already has a pattern for dynamically resolved config paths.

4. Actionable: The comment suggests concrete alternatives — using Bazel runfiles resolution or accepting the path via argv/env — and asks to apply the fix to all newly added test binaries.

5. Real impact: In sandboxed environments (Bazel CI, developer machines without root access), the hardcoded /etc/mw_com_config.json path won't exist, causing the tests to fail or use a stale/wrong config from the host rather than the bundled one.

Evidence Summary

1. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/BUILD

1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

2. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_concurrent_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

3. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_inter_stop_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

4. File Snapshot (HEAD)

• File: score/mw/com/example/ipc_bridge/find_long_running_handler_test.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...

5. File Snapshot (BASE)

• File: score/mw/com/example/ipc_bridge/BUILD

1: # *******************************************************************************
2: # Copyright (c) 2025 Contributors to the Eclipse Foundation
3: #
4: # See the NOTICE file(s) distributed with this work for additional
5: # information regarding copyright ownership.
6: #
7: # This program and the accompanying materials are made available under the
8: # terms of the Apache License Version 2.0 which is available at
9: # https://www.apache.org/licenses/LICENSE-2.0
10: #
...

6. File Snapshot (BASE)

• File: score/mw/com/example/ipc_bridge/main.cpp

1: /********************************************************************************
2:  * Copyright (c) 2025 Contributors to the Eclipse Foundation
3:  *
4:  * See the NOTICE file(s) distributed with this work for additional
5:  * information regarding copyright ownership.
6:  *
7:  * This program and the accompanying materials are made available under the
8:  * terms of the Apache License Version 2.0 which is available at
9:  * https://www.apache.org/licenses/LICENSE-2.0
10:  *
...