From c8a1f39a6c281c43fa4fc612ca36ad3474ef60d5 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Mon, 7 Sep 2026 17:28:10 +0200 Subject: [PATCH 1/5] test_rpcserver: scope handler registration to the test case A failing assertion inside a SECTION reports twice. Catch2 re-runs the TEST_CASE body once per SECTION, and the trailing test_remove_handler() never runs when a fatal REQUIRE unwinds first, so the handler survives into the next run and its add_method_handler() fails as well. The second failure points at a registration that was never the problem. Tie registration to a scope guard instead, so the handler is removed on the way out however the body exits. The concurrent-request case never removed its two handlers at all, and never checked that registering them worked; it uses the guard now as well. --- .../rpc/server/unit_tests/test_rpcserver.cc | 66 +++++++++++++++---- 1 file changed, 52 insertions(+), 14 deletions(-) diff --git a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc index c1146f43e68..65b8c2a6bc2 100644 --- a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc +++ b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc @@ -77,6 +77,42 @@ add_method_handler(const std::string &name, Func &&call) { return rpc::JsonRPCManager::instance().add_method_handler(name, std::forward(call), nullptr, {}); } + +/// Registers a method handler and removes it when the scope ends. +/// +/// Catch2 re-runs a TEST_CASE body once per leaf SECTION. Registering at the top of the body and +/// removing at the bottom only works while every assertion passes: REQUIRE is fatal, so a failure +/// inside a SECTION unwinds before the trailing removal and the handler survives into the next +/// SECTION's run, where re-registering it fails. One real failure then reports as two, and the +/// second points at a registration that was never the problem. +class ScopedMethodHandler +{ +public: + template ScopedMethodHandler(std::string name, Func &&call) : _name{std::move(name)} + { + _registered = rpc::add_method_handler(_name, std::forward(call)); + } + + ~ScopedMethodHandler() + { + if (_registered) { + rpc::test_remove_handler(_name); + } + } + + ScopedMethodHandler(ScopedMethodHandler const &) = delete; + ScopedMethodHandler &operator=(ScopedMethodHandler const &) = delete; + + bool + registered() const + { + return _registered; + } + +private: + std::string _name; + bool _registered{false}; +}; } // namespace rpc namespace @@ -424,8 +460,10 @@ TEST_CASE("Sending 'concurrent' requests to the rpc server.", "[thread]") { SECTION("A registered handlers") { - rpc::add_method_handler("some_foo", &some_foo); - rpc::add_method_handler("some_foo2", &some_foo); + rpc::ScopedMethodHandler some_foo_handler{"some_foo", &some_foo}; + rpc::ScopedMethodHandler some_foo2_handler{"some_foo2", &some_foo}; + REQUIRE(some_foo_handler.registered()); + REQUIRE(some_foo2_handler.registered()); std::promise p1; std::promise p2; @@ -480,7 +518,8 @@ DEFINE_JSONRPC_PROTO_FUNCTION(do_nothing) // id, params, resp TEST_CASE("Basic message sending to a running server", "[socket]") { - REQUIRE(rpc::add_method_handler("do_nothing", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + REQUIRE(handler.registered()); SECTION("Basic single request to the rpc server") { const int S{500}; @@ -492,7 +531,6 @@ TEST_CASE("Basic message sending to a running server", "[socket]") REQUIRE(resp == R"({"jsonrpc": "2.0", "result": {"size": ")" + std::to_string(S) + R"("}, "id": "EfGh-1"})"); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing")); } TEST_CASE("JSONRPC socket inode permissions reflect restricted_api config", "[socket][permissions]") @@ -535,7 +573,8 @@ TEST_CASE("JSONRPC socket inode permissions reflect restricted_api config", "[so TEST_CASE("Sending a message bigger than the internal server's buffer. 32000", "[buffer][error]") { - REQUIRE(rpc::add_method_handler("do_nothing32000", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing32000", &do_nothing}; + REQUIRE(handler.registered()); const int S{32000}; // + the rest of the json message. auto json{R"({"jsonrpc": "2.0", "method": "do_nothing32000", "params": {"msg":")" + random_string(S) + R"("}, "id":"32k_1"})"}; @@ -569,12 +608,12 @@ TEST_CASE("Sending a message bigger than the internal server's buffer. 32000", " REQUIRE(resp.empty()); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing32000")); } TEST_CASE("Test with invalid json message", "[socket]") { - REQUIRE(rpc::add_method_handler("do_nothing", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + REQUIRE(handler.registered()); SECTION("A rpc server") { @@ -587,12 +626,12 @@ TEST_CASE("Test with invalid json message", "[socket]") CHECK(resp == R"({"jsonrpc": "2.0", "error": {"code": -32700, "message": "Parse error"}})"); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing")); } TEST_CASE("Test with chunks", "[socket][chunks]") { - REQUIRE(rpc::add_method_handler("do_nothing", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + REQUIRE(handler.registered()); SECTION("Sending request by chunks") { @@ -609,12 +648,12 @@ TEST_CASE("Test with chunks", "[socket][chunks]") REQUIRE(resp == R"({"jsonrpc": "2.0", "result": {"size": ")" + std::to_string(S) + R"("}, "id": "chunk-parts-3"})"); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing")); } TEST_CASE("Test with chunks - disconnect after second part", "[socket][chunks]") { - REQUIRE(rpc::add_method_handler("do_nothing", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + REQUIRE(handler.registered()); SECTION("Sending request by chunks") { @@ -632,12 +671,12 @@ TEST_CASE("Test with chunks - disconnect after second part", "[socket][chunks]") REQUIRE(resp == ""); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing")); } TEST_CASE("Test with chunks - incomplete message", "[socket][chunks]") { - REQUIRE(rpc::add_method_handler("do_nothing", &do_nothing)); + rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + REQUIRE(handler.registered()); SECTION("Sending request by chunks, broken message") { @@ -655,7 +694,6 @@ TEST_CASE("Test with chunks - incomplete message", "[socket][chunks]") REQUIRE(resp == R"({"jsonrpc": "2.0", "error": {"code": -32700, "message": "Parse error"}})"); }()); } - REQUIRE(rpc::test_remove_handler("do_nothing")); } // Enable toggle From 70789d1d5616de3df3ddd1612310e8404ebe1ad7 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 10:30:07 +0200 Subject: [PATCH 2/5] test_rpcserver: report a failed handler removal Addresses review feedback. The guard's destructor discarded the result of test_remove_handler(), so a handler that went missing before scope exit would leave no trace. CHECK rather than REQUIRE because the destructor runs during unwinding when a SECTION failed, where a fatal assertion would abort instead of report. --- src/mgmt/rpc/server/unit_tests/test_rpcserver.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc index 65b8c2a6bc2..d2ad191d5cb 100644 --- a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc +++ b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc @@ -96,7 +96,9 @@ class ScopedMethodHandler ~ScopedMethodHandler() { if (_registered) { - rpc::test_remove_handler(_name); + // CHECK rather than REQUIRE: this runs during stack unwinding when a SECTION failed, and a + // fatal assertion there would abort instead of reporting. + CHECK(rpc::test_remove_handler(_name)); } } From 1e392c5c285b6663e48bd0569c37f209a08aeda5 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 11:34:44 +0200 Subject: [PATCH 3/5] test_rpcserver: name the handler in a failed cleanup, guard registered() Addresses review feedback. A failed removal reported only the expression, not which handler it was; INFO carries the name into the failure output. Mark registered() [[nodiscard]] so a caller cannot drop the registration result the guard's contract depends on. --- src/mgmt/rpc/server/unit_tests/test_rpcserver.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc index d2ad191d5cb..fb1abee0e41 100644 --- a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc +++ b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc @@ -98,6 +98,7 @@ class ScopedMethodHandler if (_registered) { // CHECK rather than REQUIRE: this runs during stack unwinding when a SECTION failed, and a // fatal assertion there would abort instead of reporting. + INFO("handler: " << _name); CHECK(rpc::test_remove_handler(_name)); } } @@ -105,7 +106,7 @@ class ScopedMethodHandler ScopedMethodHandler(ScopedMethodHandler const &) = delete; ScopedMethodHandler &operator=(ScopedMethodHandler const &) = delete; - bool + [[nodiscard]] bool registered() const { return _registered; From 0c10ae00b6bb3f10d1ca42f60196240609beaf84 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 12:17:26 +0200 Subject: [PATCH 4/5] test_rpcserver: keep the scope guard out of the rpc namespace Addresses review feedback. ScopedMethodHandler depends on Catch2 macros and has no reason to sit in the production rpc namespace, where it could collide with real RPC code. Only test_remove_handler needs to be there, because JsonRPCManager names it as a friend to reach a protected member. Guard the destructor as well. Taking the dispatcher lock or building the diagnostic can throw, and an exception leaving a destructor ends the whole test binary rather than the one assertion. --- .../rpc/server/unit_tests/test_rpcserver.cc | 39 ++++++++++++------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc index fb1abee0e41..05cd6554b95 100644 --- a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc +++ b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc @@ -43,6 +43,7 @@ #include #include #include +#include #include "swoc/swoc_file.h" @@ -77,7 +78,10 @@ add_method_handler(const std::string &name, Func &&call) { return rpc::JsonRPCManager::instance().add_method_handler(name, std::forward(call), nullptr, {}); } +} // namespace rpc +namespace +{ /// Registers a method handler and removes it when the scope ends. /// /// Catch2 re-runs a TEST_CASE body once per leaf SECTION. Registering at the top of the body and @@ -93,13 +97,21 @@ class ScopedMethodHandler _registered = rpc::add_method_handler(_name, std::forward(call)); } - ~ScopedMethodHandler() + ~ScopedMethodHandler() noexcept { - if (_registered) { - // CHECK rather than REQUIRE: this runs during stack unwinding when a SECTION failed, and a - // fatal assertion there would abort instead of reporting. + if (!_registered) { + return; + } + // CHECK rather than REQUIRE: this runs during stack unwinding when a SECTION failed, and a + // fatal assertion there would abort instead of reporting. The try/catch is for the same + // reason -- taking the dispatcher lock or building the diagnostic can throw, and an + // exception escaping here would take the whole test binary down with it. + try { INFO("handler: " << _name); CHECK(rpc::test_remove_handler(_name)); + } catch (...) { + // CHECK is unavailable here: it may be what threw. + std::cerr << "ScopedMethodHandler: exception while removing '" << _name << "'\n"; } } @@ -116,10 +128,7 @@ class ScopedMethodHandler std::string _name; bool _registered{false}; }; -} // namespace rpc -namespace -{ constexpr std::string_view rpc_test_dir_template{"ats_rpc_XXXXXX"}; constexpr std::string_view rpc_test_socket_name{"s"}; constexpr std::string_view rpc_test_lock_name{"l"}; @@ -463,8 +472,8 @@ TEST_CASE("Sending 'concurrent' requests to the rpc server.", "[thread]") { SECTION("A registered handlers") { - rpc::ScopedMethodHandler some_foo_handler{"some_foo", &some_foo}; - rpc::ScopedMethodHandler some_foo2_handler{"some_foo2", &some_foo}; + ScopedMethodHandler some_foo_handler{"some_foo", &some_foo}; + ScopedMethodHandler some_foo2_handler{"some_foo2", &some_foo}; REQUIRE(some_foo_handler.registered()); REQUIRE(some_foo2_handler.registered()); @@ -521,7 +530,7 @@ DEFINE_JSONRPC_PROTO_FUNCTION(do_nothing) // id, params, resp TEST_CASE("Basic message sending to a running server", "[socket]") { - rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + ScopedMethodHandler handler{"do_nothing", &do_nothing}; REQUIRE(handler.registered()); SECTION("Basic single request to the rpc server") { @@ -576,7 +585,7 @@ TEST_CASE("JSONRPC socket inode permissions reflect restricted_api config", "[so TEST_CASE("Sending a message bigger than the internal server's buffer. 32000", "[buffer][error]") { - rpc::ScopedMethodHandler handler{"do_nothing32000", &do_nothing}; + ScopedMethodHandler handler{"do_nothing32000", &do_nothing}; REQUIRE(handler.registered()); const int S{32000}; // + the rest of the json message. auto json{R"({"jsonrpc": "2.0", "method": "do_nothing32000", "params": {"msg":")" + random_string(S) + R"("}, "id":"32k_1"})"}; @@ -615,7 +624,7 @@ TEST_CASE("Sending a message bigger than the internal server's buffer. 32000", " TEST_CASE("Test with invalid json message", "[socket]") { - rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + ScopedMethodHandler handler{"do_nothing", &do_nothing}; REQUIRE(handler.registered()); SECTION("A rpc server") @@ -633,7 +642,7 @@ TEST_CASE("Test with invalid json message", "[socket]") TEST_CASE("Test with chunks", "[socket][chunks]") { - rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + ScopedMethodHandler handler{"do_nothing", &do_nothing}; REQUIRE(handler.registered()); SECTION("Sending request by chunks") @@ -655,7 +664,7 @@ TEST_CASE("Test with chunks", "[socket][chunks]") TEST_CASE("Test with chunks - disconnect after second part", "[socket][chunks]") { - rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + ScopedMethodHandler handler{"do_nothing", &do_nothing}; REQUIRE(handler.registered()); SECTION("Sending request by chunks") @@ -678,7 +687,7 @@ TEST_CASE("Test with chunks - disconnect after second part", "[socket][chunks]") TEST_CASE("Test with chunks - incomplete message", "[socket][chunks]") { - rpc::ScopedMethodHandler handler{"do_nothing", &do_nothing}; + ScopedMethodHandler handler{"do_nothing", &do_nothing}; REQUIRE(handler.registered()); SECTION("Sending request by chunks, broken message") From 6c9e1399a8cfd149d5c9231274341b0b9e538d38 Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Tue, 8 Sep 2026 13:23:41 +0200 Subject: [PATCH 5/5] test_rpcserver: report a cleanup exception as a test failure The catch-all added for the guarded destructor only wrote to std::cerr, so an exception during cleanup left the test reporting a pass. FAIL_CHECK is non-fatal, so it records the failure without aborting during unwinding, and catching std::exception separately carries the message. Also make the constructor explicit; the guard is meant to be built deliberately at a scope. --- src/mgmt/rpc/server/unit_tests/test_rpcserver.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc index 05cd6554b95..e8688eb0723 100644 --- a/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc +++ b/src/mgmt/rpc/server/unit_tests/test_rpcserver.cc @@ -43,7 +43,6 @@ #include #include #include -#include #include "swoc/swoc_file.h" @@ -92,7 +91,7 @@ namespace class ScopedMethodHandler { public: - template ScopedMethodHandler(std::string name, Func &&call) : _name{std::move(name)} + template explicit ScopedMethodHandler(std::string name, Func &&call) : _name{std::move(name)} { _registered = rpc::add_method_handler(_name, std::forward(call)); } @@ -102,16 +101,17 @@ class ScopedMethodHandler if (!_registered) { return; } - // CHECK rather than REQUIRE: this runs during stack unwinding when a SECTION failed, and a - // fatal assertion there would abort instead of reporting. The try/catch is for the same - // reason -- taking the dispatcher lock or building the diagnostic can throw, and an - // exception escaping here would take the whole test binary down with it. + // Non-fatal assertions throughout: this runs during stack unwinding when a SECTION failed, + // and a fatal one would abort instead of reporting. The try/catch is for the same reason -- + // taking the dispatcher lock or building the diagnostic can throw, and an exception escaping + // a destructor ends the whole test binary rather than the one assertion. try { INFO("handler: " << _name); CHECK(rpc::test_remove_handler(_name)); + } catch (std::exception const &ex) { + FAIL_CHECK("exception while removing handler '" << _name << "': " << ex.what()); } catch (...) { - // CHECK is unavailable here: it may be what threw. - std::cerr << "ScopedMethodHandler: exception while removing '" << _name << "'\n"; + FAIL_CHECK("unknown exception while removing handler '" << _name << "'"); } }