diff --git a/score/launch_manager/src/daemon/src/control/BUILD b/score/launch_manager/src/daemon/src/control/BUILD index da97a4efb..18e684eaa 100644 --- a/score/launch_manager/src/daemon/src/control/BUILD +++ b/score/launch_manager/src/daemon/src/control/BUILD @@ -11,6 +11,7 @@ # SPDX-License-Identifier: Apache-2.0 # ******************************************************************************* load("@rules_cc//cc:defs.bzl", "cc_library") +load("//tests/utils/bazel:unit_test.bzl", "lm_cc_test") cc_library( name = "control_provider", @@ -26,3 +27,21 @@ cc_library( "//score/launch_manager/src/lm_control", ], ) + +lm_cc_test( + name = "control_provider_UT", + srcs = ["control_provider_UT.cpp"], + args = [ + "--service_instance_manifest", + "$(rootpath control_provider_test_mw_com_config.json)", + ], + data = ["control_provider_test_mw_com_config.json"], + deps = [ + ":control_provider", + "//score/launch_manager:error", + "//score/launch_manager/src/daemon/src/process_group_manager:irun_target_control", + "@googletest//:gtest", + "@score_baselibs//score/string_manipulation/arguments", + "@score_communication//score/mw/com", + ], +) diff --git a/score/launch_manager/src/daemon/src/control/control_provider.cpp b/score/launch_manager/src/daemon/src/control/control_provider.cpp index e30923cdf..b84cfd7fc 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.cpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.cpp @@ -18,7 +18,7 @@ namespace score::mw::lifecycle::internal { -Result ControlProvider::Create(IRunTargetControl* graph) noexcept +Result> ControlProvider::Create(IRunTargetControl* graph) noexcept { const Result instance_specifier_result = com::InstanceSpecifier::Create(std::string{"LaunchManager/StateManager/Instance"}); @@ -36,7 +36,9 @@ Result ControlProvider::Create(IRunTargetControl* graph) noexc } LmControlSkeleton skeleton = std::move(skeleton_result).value(); - auto* control_provider = new ControlProvider{std::move(skeleton), graph}; + // Owned by a `unique_ptr` so that it is freed on any of the failure paths below. Not + // `std::make_unique`, because the constructor is private. + std::unique_ptr control_provider{new ControlProvider{std::move(skeleton), graph}}; const Result setup_activate_run_target_result = control_provider->setupActivateRunTarget(); if (!setup_activate_run_target_result.has_value()) diff --git a/score/launch_manager/src/daemon/src/control/control_provider.hpp b/score/launch_manager/src/daemon/src/control/control_provider.hpp index bb976e575..58664ae92 100644 --- a/score/launch_manager/src/daemon/src/control/control_provider.hpp +++ b/score/launch_manager/src/daemon/src/control/control_provider.hpp @@ -17,17 +17,20 @@ #include "score/mw/launch_manager/process_group_manager/irun_target_control.hpp" #include "score/mw/lifecycle/details/lm_control_service.h" +#include + namespace score::mw::lifecycle::internal { /// @brief Provides the mw::com service for state managers to connect to. /// @details This cannot be moved, because the mw::com callbacks reference -// the ControlProvider at its original location. +// the ControlProvider at its original location. It is therefore +// handed out as a `std::unique_ptr`, which keeps that address fixed. class ControlProvider { public: /// @brief Fallible constructor for ControllableGraph. - static Result Create(IRunTargetControl* graph) noexcept; + static Result> Create(IRunTargetControl* graph) noexcept; ~ControlProvider() = default; diff --git a/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp new file mode 100644 index 000000000..d256f748d --- /dev/null +++ b/score/launch_manager/src/daemon/src/control/control_provider_UT.cpp @@ -0,0 +1,133 @@ +/******************************************************************************** + * Copyright (c) 2026 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 + ********************************************************************************/ + +#include "score/mw/launch_manager/control/control_provider.hpp" +#include "score/mw/launch_manager/process_group_manager/irun_target_control.hpp" + +#include "score/mw/com/runtime.h" +#include "score/string_manipulation/arguments/arguments.h" + +#include + +#include +#include +#include + +namespace score::mw::lifecycle::internal +{ +namespace +{ + +// Locks in the contract from the class comment in control_provider.hpp: the registered callbacks +// capture the ControlProvider's own address, so it must stay neither movable nor copyable, and +// is handed out as a `std::unique_ptr` instead. A regression here (e.g. someone defaulting the +// move ops) would silently reintroduce a dangling-callback bug. +static_assert(!std::is_move_constructible_v); +static_assert(!std::is_move_assignable_v); +static_assert(!std::is_copy_constructible_v); +static_assert(!std::is_copy_assignable_v); + +// `Create()` never calls back into `graph` before returning, so a no-op stub is sufficient for +// `getActiveRunTarget`/`setRequestedRunTarget`. `registerActiveRunTargetCallback` is captured so +// the test below can invoke it directly, as if a real transition had just completed. +class FakeRunTargetControl : public IRunTargetControl +{ + public: + [[nodiscard]] score::Result getActiveRunTarget() const noexcept override + { + return MakeUnexpected(ExecErrc::kCommunicationError); + } + + [[nodiscard]] score::Result setRequestedRunTarget(IdentifierHash /*run_target*/) noexcept override + { + return {}; + } + + void registerActiveRunTargetCallback(ActivationCallbackT callback) noexcept override + { + callback_ = std::move(callback); + } + + void TriggerActivation(IdentifierHash state, RunTargetActivationSource source) + { + ASSERT_TRUE(static_cast(callback_)) << "registerActiveRunTargetCallback was never called"; + callback_(state, source); + } + + private: + ActivationCallbackT callback_; +}; + +class ControlProviderUT : public ::testing::Test +{ + protected: + FakeRunTargetControl graph_; +}; + +TEST_F(ControlProviderUT, StaticAssertionsCompiled) +{ + // The interesting assertions above run at compile time; this keeps the test target from + // being empty and gives CI something to report. + SUCCEED(); +} + +TEST_F(ControlProviderUT, CallbacksDispatchAfterOwnershipTransfer) +{ + RecordProperty( + "Description", + "After the std::unique_ptr returned by Create() is moved to a different owner, the " + "registered callbacks still dispatch through the same, still-alive ControlProvider."); + + const IdentifierHash run_target_id{"control_provider_ut_run_target"}; + + std::unique_ptr owner; + { + Result> create_result = ControlProvider::Create(&graph_); + ASSERT_TRUE(create_result.has_value()); + + // Only the pointer moves; the ControlProvider the callbacks captured stays where it is. + owner = std::move(create_result).value(); + } + ASSERT_NE(owner, nullptr); + + // Doesn't crash and doesn't trip ASan/UBSan iff the callback dispatches through the + // still-alive ControlProvider that `owner` now holds. + graph_.TriggerActivation(run_target_id, RunTargetActivationSource::kStateManagerRequest); +} + +TEST_F(ControlProviderUT, DestructionReleasesService) +{ + RecordProperty( + "Description", + "Destroying the ControlProvider returned by Create() releases the offered service, so " + "a later Create() for the same instance succeeds again."); + + Result> first = ControlProvider::Create(&graph_); + ASSERT_TRUE(first.has_value()); + first.value().reset(); + + FakeRunTargetControl second_graph; + Result> second = ControlProvider::Create(&second_graph); + EXPECT_TRUE(second.has_value()); +} + +} // namespace +} // namespace score::mw::lifecycle::internal + +int main(int argc, char** argv) +{ + ::testing::InitGoogleTest(&argc, argv); + score::mw::com::runtime::InitializeRuntime( + score::string_manipulation::GetArguments(argc, const_cast(argv))); + return RUN_ALL_TESTS(); +} diff --git a/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json b/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json new file mode 100644 index 000000000..861d110c6 --- /dev/null +++ b/score/launch_manager/src/daemon/src/control/control_provider_test_mw_com_config.json @@ -0,0 +1,70 @@ +{ + "serviceTypes": [ + { + "serviceTypeName": "/score/mw/lifecycle/LmControlService", + "version": { + "major": 1, + "minor": 0 + }, + "bindings": [ + { + "binding": "SHM", + "serviceId": 7101, + "events": [ + { + "eventName": "ActivationResult", + "eventId": 1 + } + ], + "methods": [ + { + "methodName": "ActivateRunTarget", + "methodId": 2 + }, + { + "methodName": "GetActiveRunTarget", + "methodId": 3 + } + ] + } + ] + } + ], + "serviceInstances": [ + { + "instanceSpecifier": "LaunchManager/StateManager/Instance", + "serviceTypeName": "/score/mw/lifecycle/LmControlService", + "version": { + "major": 1, + "minor": 0 + }, + "instances": [ + { + "instanceId": 1, + "asil-level": "QM", + "binding": "SHM", + "events": [ + { + "eventName": "ActivationResult", + "numberOfSampleSlots": 8, + "maxSubscribers": 1 + } + ], + "methods": [ + { + "methodName": "ActivateRunTarget", + "queueSize": 1 + }, + { + "methodName": "GetActiveRunTarget", + "queueSize": 1 + } + ] + } + ] + } + ], + "global": { + "asil-level": "QM" + } +} diff --git a/score/launch_manager/src/daemon/src/run.cpp b/score/launch_manager/src/daemon/src/run.cpp index eb390ebcc..80720022d 100644 --- a/score/launch_manager/src/daemon/src/run.cpp +++ b/score/launch_manager/src/daemon/src/run.cpp @@ -175,10 +175,19 @@ int run(int argc, const char* argv[]) std::move(watchdog), config_result.value().takeWatchdog()); + // Declared here, outside the `if` block below, and deliberately not destroyed until + // after `deinitialize()` returns: `deinitialize()`'s own comment notes that a worker may + // still be (de)activating a node when it runs, and drains those in flight before + // resetting `graph_`. A drain that completes a transition calls + // Graph::finalizeTransitionSuccess(), which invokes the very callback `ControlProvider` + // registered via `registerActiveRunTargetCallback`. If `ControlProvider` were destroyed + // before that drain finishes -- as it would be if scoped only to the `if` block below -- + // that callback would fire through a dangling pointer. + std::unique_ptr control_provider; + if (process_group_manager->initialize()) { - // Remains active in the background until the ControlProvider is destroyed. - const score::Result control_provider_result = + score::Result> control_provider_result = ControlProvider::Create(process_group_manager.get()); if (!control_provider_result.has_value()) @@ -187,9 +196,13 @@ int run(int argc, const char* argv[]) << control_provider_result.error().Message(); exit_code = EXIT_FAILURE; } - else if (runLCMDaemon(*process_group_manager)) + else { - exit_code = EXIT_SUCCESS; + control_provider = std::move(control_provider_result).value(); + if (runLCMDaemon(*process_group_manager)) + { + exit_code = EXIT_SUCCESS; + } } } @@ -198,6 +211,9 @@ int run(int argc, const char* argv[]) process_group_manager->deinitialize(); process_group_manager.reset(); } + + // Safe to let `control_provider` go out of scope now: `deinitialize()` above has + // already joined every worker and reset `graph_`, so no further callback can arrive. } catch (...) {