fix(service-registry): make the convenience registerService honor its documented assertion

CHANGELOG.md

@@ -5,6 +5,17 @@ All notable changes to `plotjuggler_sdk` are recorded here. Versioning policy is
 
 ## [0.21.0]
 
+### Fix: convenience registerService honors its documented assertion (PATCH)
+
+The non-returning `ServiceRegistryBuilder::registerService` overloads documented
+a debug assertion but discarded the `tryRegisterService` status in every build
+type, so duplicate or null registrations were silent. A rejection now trips
+`PJ_ASSERT`, and the doc-comments state the exact NDEBUG behavior (status
+dropped; callers that must report failures use `tryRegisterService`). Adds the
+first regression tests for the builder's registration rules
+(`service_registry_builder_test`, built with `PJ_ASSERT_THROWS` so the
+invariant is observable in every build type).
+
 ### Feature: pure-functional MessageParser C extension (MINOR)
 
 MessageParser scalar/object results no longer require new hosts to cast an

pj_plugins/CMakeLists.txt

@@ -344,6 +344,18 @@ target_link_libraries(object_ingest_policy_test PRIVATE
 target_include_directories(object_ingest_policy_test PRIVATE include)
 add_test(NAME object_ingest_policy_test COMMAND object_ingest_policy_test)
 
+# Unit test: ServiceRegistryBuilder registration rules. PJ_ASSERT_THROWS makes
+# the convenience overload's invariant observable in every build type — a plain
+# assert() is compiled away under NDEBUG, which is where it would go unchecked.
+add_executable(service_registry_builder_test tests/service_registry_builder_test.cpp)
+target_compile_definitions(service_registry_builder_test PRIVATE PJ_ASSERT_THROWS)
+target_compile_options(service_registry_builder_test PRIVATE ${PJ_WARNING_FLAGS})
+target_link_libraries(service_registry_builder_test PRIVATE
+  pj_base GTest::gtest_main
+)
+target_include_directories(service_registry_builder_test PRIVATE include)
+add_test(NAME service_registry_builder_test COMMAND service_registry_builder_test)
+
 # TODO(v3-port): delegated_ingest_integration_test.cpp uses old bindWriteHost /
 # bindRuntimeHost methods and get_last_error slots removed in v3. Pending port
 # to the service registry + PJ_error_t* pattern. Its coverage (parser binding +

pj_plugins/include/pj_plugins/host/service_registry_builder.hpp

@@ -9,6 +9,7 @@
 #include <unordered_map>
 #include <utility>
 
+#include "pj_base/assert.hpp"
 #include "pj_base/expected.hpp"
 #include "pj_base/plugin_data_api.h"
 
@@ -58,16 +59,21 @@ class ServiceRegistryBuilder {
   }
 
   /// Non-returning convenience overload for callers that know the inputs are
-  /// valid (mocks, tests). Asserts in debug builds; no-op on failure in
-  /// release (i.e. do NOT rely on this for untrusted inputs — use
-  /// tryRegisterService instead).
+  /// valid (mocks, tests). A rejection trips `PJ_ASSERT`, which aborts while
+  /// `assert()` is live and throws under `PJ_ASSERT_THROWS` — but NDEBUG
+  /// compiles it away, so in a release build the registration is silently
+  /// dropped and the reason is lost. Anything whose inputs are not statically
+  /// known must call tryRegisterService and handle the Status.
   void registerService(std::string_view name, uint32_t protocol_version, PJ_service_t service) {
-    auto status = tryRegisterService(name, protocol_version, service);
-    (void)status;
+    [[maybe_unused]] const ::PJ::Status status = tryRegisterService(name, protocol_version, service);
+    PJ_ASSERT(status.has_value(), "registerService: rejected (duplicate name, or null ctx/vtable)");
   }
 
   /// Typed overload using a service-traits class (see sdk/service_traits.hpp).
   /// The traits provide the canonical name and a default protocol version.
+  /// Inherits the overload above's release-build behavior: a rejection is
+  /// dropped without a trace, so a host that must report it registers through
+  /// tryRegisterService with the same expansion.
   template <class Traits>
   void registerService(typename Traits::Raw service) {
     registerService(

pj_plugins/tests/service_registry_builder_test.cpp

@@ -0,0 +1,91 @@
+// Copyright 2026 Davide Faconti
+// SPDX-License-Identifier: Apache-2.0
+
+#include "pj_plugins/host/service_registry_builder.hpp"
+
+#include <gtest/gtest.h>
+
+#include <stdexcept>
+#include <string>
+
+#include "pj_base/plugin_data_api.h"
+#include "pj_base/sdk/service_traits.hpp"
+
+// This TU is compiled with PJ_ASSERT_THROWS so the convenience overload's
+// invariant is observable in every build type (a plain assert() is compiled
+// away under NDEBUG, which is exactly how the missing check went unnoticed).
+#ifndef PJ_ASSERT_THROWS
+#error "service_registry_builder_test must be built with PJ_ASSERT_THROWS"
+#endif
+
+namespace {
+
+// The builder only stores the fat pointer; it never dereferences the vtable,
+// so a zeroed one is enough to make a service "valid".
+const PJ_source_write_host_vtable_t kWriteVtable{};
+int ctx_a = 0;
+int ctx_b = 0;
+
+PJ_source_write_host_t makeWriteHost(int* ctx) {
+  return PJ_source_write_host_t{ctx, &kWriteVtable};
+}
+
+constexpr const char* kName = PJ::sdk::SourceWriteHostService::kName;
+
+TEST(ServiceRegistryBuilderTest, FirstRegistrationSucceeds) {
+  PJ::ServiceRegistryBuilder builder;
+
+  const PJ::Status status = builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable});
+
+  EXPECT_TRUE(status.has_value()) << status.error();
+  EXPECT_EQ(builder.size(), 1U);
+}
+
+TEST(ServiceRegistryBuilderTest, DuplicateNameIsRejectedAndKeepsTheFirstEntry) {
+  PJ::ServiceRegistryBuilder builder;
+  ASSERT_TRUE(builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable}).has_value());
+
+  const PJ::Status status = builder.tryRegisterService(kName, 2, PJ_service_t{&ctx_b, &kWriteVtable});
+
+  ASSERT_FALSE(status.has_value());
+  EXPECT_NE(status.error().find("duplicate name"), std::string::npos) << status.error();
+  EXPECT_NE(status.error().find(kName), std::string::npos) << status.error();
+  EXPECT_EQ(builder.size(), 1U);
+}
+
+TEST(ServiceRegistryBuilderTest, NullCtxOrVtableIsRejected) {
+  PJ::ServiceRegistryBuilder builder;
+
+  EXPECT_FALSE(builder.tryRegisterService(kName, 1, PJ_service_t{nullptr, &kWriteVtable}).has_value());
+  EXPECT_FALSE(builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, nullptr}).has_value());
+  EXPECT_EQ(builder.size(), 0U);
+}
+
+// The convenience overload documents that it asserts; without that assert a
+// duplicate silently changes the service surface a plugin binds against.
+TEST(ServiceRegistryBuilderTest, ConvenienceOverloadAssertsOnDuplicate) {
+  PJ::ServiceRegistryBuilder builder;
+  builder.registerService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable});
+
+  EXPECT_THROW(builder.registerService(kName, 1, PJ_service_t{&ctx_b, &kWriteVtable}), std::runtime_error);
+  EXPECT_EQ(builder.size(), 1U);
+}
+
+TEST(ServiceRegistryBuilderTest, ConvenienceOverloadAssertsOnNullService) {
+  PJ::ServiceRegistryBuilder builder;
+
+  EXPECT_THROW(builder.registerService(kName, 1, PJ_service_t{nullptr, nullptr}), std::runtime_error);
+  EXPECT_EQ(builder.size(), 0U);
+}
+
+// The templated path is the one every host actually calls, so it must inherit
+// the same invariant rather than quietly dropping the second registration.
+TEST(ServiceRegistryBuilderTest, TemplatedOverloadAssertsOnDuplicate) {
+  PJ::ServiceRegistryBuilder builder;
+  builder.registerService<PJ::sdk::SourceWriteHostService>(makeWriteHost(&ctx_a));
+
+  EXPECT_THROW(builder.registerService<PJ::sdk::SourceWriteHostService>(makeWriteHost(&ctx_b)), std::runtime_error);
+  EXPECT_EQ(builder.size(), 1U);
+}
+
+}  // namespace